Hello @zigbang-rad, I'm Gemini Code Assist¹! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

Summary of Changes

This pull request addresses deprecation issues in iOS 18 by updating the cordova-plugin-inappbrowser plugin. The primary change involves modifying how URLs are opened in the system, using the openURL:options:completionHandler: method for better compatibility and handling of potential failures. Additionally, the plugin version is bumped from 5.0.20-hogangnono to 5.0.24-hogangnono.

Highlights

• iOS 18 Compatibility: The code changes ensure compatibility with iOS 18 by addressing deprecated methods related to opening URLs.
• URL Opening Mechanism: The openURL method is updated to use the openURL:options:completionHandler: method, providing a completion handler to manage success or failure scenarios.
• Version Bump: The plugin version is updated to 5.0.24-hogangnono in package-lock.json, package.json, and plugin.xml.

Changelog

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

An iOS upgrade,
A version bump takes place now,
Bugs fade into mist.

Code Review

This pull request updates the cordova-plugin-inappbrowser plugin to address deprecation issues in iOS 18. The changes primarily involve updating the plugin version and using the openURL:options:completionHandler: method for opening URLs in the system. Overall, the changes seem reasonable and address the intended issue.

Summary of Findings

• Potential issue with openURL fallback: In CDVWKInAppBrowser.m, the fallback mechanism using NSNotificationCenter might not be necessary with the new openURL:options:completionHandler: API. It's worth verifying if this fallback is still needed and if it behaves as expected.
• Inconsistency in openURL usage: In CDVWKInAppBrowser.m, the openURL method is called with options:@{} and a completion handler. However, in SVWebViewControllerActivityChrome.m, SVWebViewControllerActivitySafari.m, the completion handler is set to nil. While this might not be an issue, it's worth ensuring consistency in how the API is used across the codebase.

Merge Readiness

The pull request appears to address the intended issue of iOS 18 deprecation. However, before merging, it's recommended to verify the behavior of the openURL fallback in CDVWKInAppBrowser.m and ensure consistency in the usage of openURL:options:completionHandler: across all files. I am unable to approve this pull request, and recommend that others review and approve this code before merging.

Auto Pull Request Review from LlamaPReview

1. Overview

1.1 Core Changes

• Primary purpose and scope: This PR aims to update the cordova-plugin-inappbrowser to ensure compatibility with iOS 18 by addressing deprecated openURL APIs. It also includes a version bump for the plugin.
• Key components modified:
  • src/ios/CDVWKInAppBrowser.m: Updated URL opening mechanisms.
  • src/ios/SVWebViewControllerActivityChrome.m: Updated URL opening.
  • src/ios/SVWebViewControllerActivitySafari.m: Updated URL opening.
  • package.json, package-lock.json, plugin.xml: Version updates.
  • src/osx/CDVInAppBrowser.m: Removed (macOS support dropped).
• Cross-component impacts: The changes primarily affect iOS functionality. The removal of macOS code implies that platform is no longer supported by this plugin version.
• Business value alignment: Ensures continued functionality and compatibility of applications using this plugin on the upcoming iOS 18, preventing potential issues or App Store rejections related to deprecated API usage.

1.2 Technical Architecture

• System design modifications: The method for opening external URLs on iOS has been updated from the deprecated openURL: to the newer openURL:options:completionHandler:. This allows for asynchronous handling and better feedback on the success or failure of the operation.
• Component interaction changes: Interactions with UIApplication for URL opening are modernized.
• Integration points impact: Apps relying on this plugin for InAppBrowser functionality on iOS will use the updated API.
• Dependency changes and implications: No new external dependencies are added. The plugin now relies on iOS SDK features available in versions that support openURL:options:completionHandler: (iOS 10+). macOS support has been removed.

2. Critical Findings

2.1 Must Fix (P0🔴)

Issue: Typo in UIApplication method call for opening URLs in error handling path.

• Analysis Confidence: High
• Impact: This will lead to a compilation error or a runtime crash when attempting to open a URL in the specific error handling scenario (e.g., NSURLErrorUnsupportedURL), as open:options:completionHandler: is not a valid UIApplication method. The intended method is openURL:options:completionHandler:.
• Resolution: In src/ios/CDVWKInAppBrowser.m, change [[UIApplication sharedApplication] open:url options:@{} completionHandler:...] back to [[UIApplication sharedApplication] openURL:url options:@{} completionHandler:...] within the webView:didFailNavigation:withError: method.

Issue: Plugin metadata (plugin.xml) likely still lists macOS as a supported platform despite code removal.

• Analysis Confidence: High (based on deep analysis; not directly visible in provided plugin.xml diff, which only shows version change)
• Impact: Misleads users about platform compatibility. Cordova tooling might attempt to install macOS components that no longer exist, potentially causing build issues or confusion.
• Resolution: Remove the <platform name="osx"> (or similar) entry from the <platforms> tag within plugin.xml to accurately reflect that macOS is no longer supported.

2.2 Should Fix (P1🟡)

Issue: Incomplete error handling for URL opening in Chrome custom activity.

• Analysis Confidence: High
• Impact: If opening a URL in Chrome via SVWebViewControllerActivityChrome.m fails, the failure is silently ignored as completionHandler:nil is used. This can make debugging difficult and provide a poor user experience if an action doesn't complete as expected.
• Suggested Solution: Implement the completion handler for openURL:options:completionHandler: in src/ios/SVWebViewControllerActivityChrome.m to log errors or provide feedback if success is NO.

Issue: Multiple version bumps in quick succession.

• Analysis Confidence: Medium
• Impact: The plugin version was bumped from 5.0.20 to 5.0.22, then to 5.0.23, and finally to 5.0.24 across several commits. This can create confusion for consumers tracking plugin versions and may indicate instability during the development of this feature.
• Suggested Solution: Consolidate changes under a single, meaningful version increment if possible before merging. If these represent distinct, tested stages, ensure changelogs are very clear. For this PR, ensure 5.0.24-hogangnono is the final, correct version.

Issue: Potential for improved error diagnostics in Safari custom activity.

• Analysis Confidence: Medium
• Impact: While SVWebViewControllerActivitySafari.m uses the success status from the completion handler to call activityDidFinish:, it doesn't explicitly log or handle the failure case before this call. This could make diagnosing specific URL opening failures less direct.
• Suggested Solution: In src/ios/SVWebViewControllerActivitySafari.m, consider adding a log statement if success is NO within the completion handler, before calling [self activityDidFinish:success], to aid in debugging.

2.3 Consider (P2🟢)

Area: Backward Compatibility for older iOS versions.

• Analysis Confidence: Medium
• Improvement Opportunity: The openURL:options:completionHandler: API was introduced in iOS 10. If this plugin needs to support iOS versions older than 10 (though unlikely for an iOS 18 compatibility update), a runtime check using @available(iOS 10.0, *) would be necessary to use the old openURL: for those versions. This PR seems to target modern iOS, so this might be a low priority.

Area: Documentation and CI/CD for macOS support removal.

• Analysis Confidence: Medium
• Improvement Opportunity:
  • Explicitly document the removal of macOS support in the plugin's README or changelog.
  • Update any CI/CD pipelines that might still attempt to build or test for macOS.

Area: Objective-C Nullability Annotations.

• Analysis Confidence: Low
• Improvement Opportunity: For improved Swift interoperability and code clarity, consider adding nullability annotations (e.g., nullable, nonnull) to Objective-C method parameters and return types in the modified files.

2.4 Summary of Action Items

• P0 (Must Fix - Before Merge):
  1. Correct the open: typo to openURL: in CDVWKInAppBrowser.m.
  2. Update plugin.xml to remove macOS from supported platforms.
• P1 (Should Fix - Ideally Before Merge):
  1. Implement error handling in SVWebViewControllerActivityChrome.m's completion handler.
  2. Review versioning strategy; ensure 5.0.24-hogangnono is the intended final version for these changes.
  3. Add logging for failed URL openings in SVWebViewControllerActivitySafari.m.
• P2 (Consider - At Discretion):
  1. Evaluate need for iOS < 10 backward compatibility.
  2. Document macOS removal and update CI.
  3. Add nullability annotations.

3. Technical Analysis

3.1 Code Logic Analysis

📁 src/ios/CDVWKInAppBrowser.m - openInSystem:

• Submitted PR Code:

    - (void)openInSystem:(NSURL*)url
    {
    -    if ([[UIApplication sharedApplication] openURL:url] == NO) {
    -        [[NSNotificationCenter defaultCenter] postNotification:[NSNotification notificationWithName:CDVPluginHandleOpenURLNotification object:url]];
    -        [[UIApplication sharedApplication] openURL:url];
    -    }
    +    [[UIApplication sharedApplication] openURL:url options:@{} completionHandler:^(BOOL success) {
    +        if (!success) {
    +            [[NSNotificationCenter defaultCenter] postNotification:[NSNotification notificationWithName:CDVPluginHandleOpenURLNotification object:url]];
    +        }
    +    }];
    }

• Analysis:
  • Current logic correctly updates the deprecated openURL: call to openURL:options:completionHandler:.
  • It uses a completion handler to check for success and posts a notification if opening the URL fails. This is an improvement over the previous synchronous check.
  • No issues identified in this specific change; it aligns with iOS 18 compatibility requirements.
• LlamaPReview Suggested Improvements: [Use comment to highlight where is improved in the code, skip this section if no improvement]

    // No improvement needed for this specific block; change is good.

📁 src/ios/CDVWKInAppBrowser.m - webView:didFailNavigation:withError:

• Submitted PR Code:

    // Snippet from the diff showing the problematic line
    if (error.code == NSURLErrorUnsupportedURL && error.userInfo[NSURLErrorFailingURLStringErrorKey]) {
        NSURL *url = [NSURL URLWithString:error.userInfo[NSURLErrorFailingURLStringErrorKey]];
        if (url) {
    -            [[UIApplication sharedApplication] openURL:url options:@{} completionHandler:^(BOOL success) {
    +            [[UIApplication sharedApplication] open:url options:@{} completionHandler:^(BOOL success) { // P0: Typo 'open:' should be 'openURL:'
                if(success) {
                    CDVPluginResult* pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK];
                    [self.commandDelegate sendPluginResult:pluginResult callbackId:self.callbackId];
                } else {
                    errorHandler([url absoluteString], self.callbackId);
                }
            }];
        }
    }

• Analysis:
  • Current logic attempts to open a URL using [[UIApplication sharedApplication] open:url options:@{} completionHandler:...].
  • Potential Issue: open:options:completionHandler: is not a valid method on UIApplication. The correct method is openURL:options:completionHandler:. This will cause a compile-time error or a runtime crash.
  • This is a critical regression introduced during the refactoring.
• LlamaPReview Suggested Improvements:

            [[UIApplication sharedApplication] openURL:url options:@{} completionHandler:^(BOOL success) { // Corrected: 'openURL:' instead of 'open:'
                if(success) {
                    CDVPluginResult* pluginResult = [CDVPluginResult resultWithStatus:CDVCommandStatus_OK];
                    [self.commandDelegate sendPluginResult:pluginResult callbackId:self.callbackId];
                } else {
                    errorHandler([url absoluteString], self.callbackId);
                }
            }];

• Improvement rationale:
  • Technical benefits: Fixes a critical error, preventing crashes and ensuring the intended functionality of opening unsupported URLs in an external browser.
  • Business value: Maintains application stability and user experience.
  • Risk assessment: Currently high risk of failure; fix is low risk and essential.

📁 src/ios/SVWebViewControllerActivityChrome.m - performActivity

• Submitted PR Code:

    // Open the URL with Chrome.
    -		[[UIApplication sharedApplication] openURL:chromeURL];
    +		[[UIApplication sharedApplication] openURL:chromeURL options:@{} completionHandler:nil]; // P1: completionHandler is nil

---
💡 **LlamaPReview Community**
Have feedback on this AI Code review tool? Join our [GitHub Discussions](https://github.com/JetXu-LLM/LlamaPReview-site/discussions) to share your thoughts and help shape the future of LlamaPReview.